Skip to content

remove let this = self; in compiler #148955

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

remove let this = self; in compiler #148955

wants to merge 1 commit into from
+347 −363

Conversation

@Kivooeo
Copy link
Member

@Kivooeo Kivooeo commented Nov 14, 2025

I still kept this as name for closure argument

I don't know original story why this was let this = self but I don't see any problems to remove it

by the way, there is one place, where it's actually necessary and provides a good solution because of macro https://github.com/rust-lang/rust/blob/main/compiler/rustc_parse/src/parser/expr.rs#L495

@rustbot
Copy link
Collaborator

rustbot commented Nov 14, 2025

Some changes occurred in match lowering

cc @Nadrieril

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Nov 14, 2025
@rustbot
Copy link
Collaborator

rustbot commented Nov 14, 2025

r? @saethlin

rustbot has assigned @saethlin.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

@bors
Copy link
Collaborator

bors commented Nov 15, 2025

☔ The latest upstream changes (presumably #148706) made this pull request unmergeable. Please resolve the merge conflicts.

@bjorn3
Copy link
Member

bjorn3 commented Nov 15, 2025

For at least one case the method used to be a closure passed to self.in_scope(), with the closure argument being this: 066d44b?w=1#diff-e63c37c3db7192043330a40f97b301858cdba917d19dd968334f30ffdae34f9eR49

Zalathar
Zalathar reviewed Nov 16, 2025
View reviewed changes
compiler/rustc_mir_build/src/builder/block.rs
Comment on lines +73 to 76
block = self
.in_scope(si, LintLevel::Inherited, |this| {
this.stmt_expr(block, *expr, Some(*scope))
})
Copy link
Member

@Zalathar Zalathar Nov 16, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See how the code inside this closure uses this, but the code outside the closure uses self?

Being able to consistently use this in both places, regardless of whether they're nested in a closure, is the whole point of doing let this = self. So getting rid of it would make the code harder to read and harder to modify.

@Zalathar
Copy link
Member

Zalathar commented Nov 16, 2025

let this = self is a widely-used idiom throughout the MIR building code, because there are a lot of calls that need to borrow self and then re-expose it through a closure argument |this|.

Switching back to self at the top level creates inconsistency between code that happens to be outside or inside one of those closures. And it creates more churn when moving code in or out of a closure, which is a reasonably common thing to need to do.

So I don't think this PR should be merged.

@Kivooeo
Copy link
Member Author

Kivooeo commented Nov 16, 2025

I'd agree if it wasn't like that in 7 places, using |this| to pass self in closure is a common practice overall in compiler and does not requires let this = self for this

for inconsistency part I don't agree because as I said places where self passed to closure like |this| way more common than let this = self

Here I want to show how inconsistent current version is (for info, there is no reason to use let this = self here, it does not affect anything, I assume CI passed means that I rewrote it correctly and logic wasn't touched)

This function uses self on top level in MIR building

rust/compiler/rustc_mir_build/src/builder/block.rs

Lines 12 to 31 in e1a2ec6

impl<'a, 'tcx> Builder<'a, 'tcx> {
pub(crate) fn ast_block(
&mut self,
destination: Place<'tcx>,
block: BasicBlock,
ast_block: BlockId,
source_info: SourceInfo,
) -> BlockAnd<()> {
let Block { region_scope, span, ref stmts, expr, targeted_by_break, safety_mode: _ } =
self.thir[ast_block];
self.in_scope((region_scope, source_info), LintLevel::Inherited, move |this| {
if targeted_by_break {
this.in_breakable_scope(None, destination, span, |this| {
Some(this.ast_block_stmts(destination, block, span, stmts, expr, region_scope))
})
} else {
this.ast_block_stmts(destination, block, span, stmts, expr, region_scope)
}
})
}

And this function uses let this = self right down below in the same file

rust/compiler/rustc_mir_build/src/builder/block.rs

Lines 33 to 42 in e1a2ec6

fn ast_block_stmts(
&mut self,
destination: Place<'tcx>,
mut block: BasicBlock,
span: Span,
stmts: &[StmtId],
expr: Option<ExprId>,
region_scope: Scope,
) -> BlockAnd<()> {
let this = self;

And here is some more examples that not rename self for this

rust/compiler/rustc_mir_build/src/check_unsafety.rs

Line 291 in e1a2ec6

fn visit_block(&mut self, block: &'a Block) {

rust/compiler/rustc_mir_build/src/builder/matches/mod.rs

Line 413 in e1a2ec6

pub(crate) fn lower_match_arms(

rust/compiler/rustc_mir_build/src/thir/pattern/check_match.rs

Line 113 in e1a2ec6

fn visit_arm(&mut self, arm: &'p Arm<'tcx>) {

(And all other function in check_match.rs)

Moreover, if I remember it correctly, we are not allowing renaming self in arguments like for this reason? Just to keep code more readable and consistent, but this is opposite

The original idea for this PR comes from that I was reading this code that renaming self and it was hard to read

tldr; around 500 usages of |this| without let this = self and 7 with it does not make sense

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@Zalathar Zalathar Zalathar left review comments

Assignees

@saethlin saethlin

Labels

S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@Kivooeo @rustbot @bors @bjorn3 @Zalathar @saethlin